fix(songbook): Fix the songbook's order, chords, repeats and front matter - #464

@claude review

Claude finished @ioanlucut's task in 2m 20s —— View job

Review

• Gather context (PR diff, changed files)
• Review LaTeX template / config changes
• Review convertToSongbookTex.ts and songToLeadsheetConverter.ts
• Review tests and docs updates
• Post final review feedback

I read the diff (git diff origin/main...HEAD), the two before/after screenshots in the PR body, and cross-checked ~250 occurrences of /:/:/ in leadsheets/ to confirm the repeat-mark rewrite only ever sees them as standalone tokens (surrounded by spaces), matching the code's assumption.

Findings: none blocking.

Everything I checked lines up with the stated intent:

• rewriteRepeatMarkAsRepeatBar (src/songToLeadsheetConverter.ts:157-164) only fires on a word that's exactly /: or :/, which matches how every occurrence in leadsheets/ and verified/ actually appears (space-delimited). The step runs after escapeTexAroundChords, so a real | typed in lyrics is escaped to \textbar{} first and never collides with the |:/:| this step emits — order is correct, confirmed by the new spec cases (songToLeadsheetConverter.spec.ts:214-228).
• Dropping rewriteNotationsWithDashForChordsWithBass and keeping ^{D/F#} as-is is consistent everywhere: converter, spec, docs (docs/leadsheets-and-songbook.md), and both skills/bes-song-leadsheets/references/*.md files were all updated together — no stale mention of the old dash rewrite remains.
• compareSongbookOrder (src/songToLeadsheetConverter.ts:190-200) reads cleanly: title → alternative → composer → arranger → band → genre → version, each ?? EMPTY_STRING-guarded since all but title are optional on SongAST, short-circuiting via truthy/falsy localeCompare results — standard and correct. Full ICU (needed for the 'ro' collation, e.g. ș/ț/ă ordering) ships by default in Node ≥13 official builds, so this should behave the same in CI as documented.
• LaTeX/songbook/bes-songbook-config.tex: all \DeclareTranslation{English}{...} entries were moved to Romanian, with no leftover English declarations — matches \setdefaultlanguage{romanian} added via polyglossia in the template.
• bes-songbook.template.txt: single Cuprins heading (old duplicate \addcontentsline + centered Cuprins block removed), \the\year replaces hard-coded 2025 in both the footer and title page, align-chords = l / chords/format = \chordfont\bfseries with the \IfFontExistsTF{MonoLisaCode} fallback to \sffamily for CI — all present as described. The before/after screenshots confirm this visually (left-aligned bold chords, single ToC entry, 29 septembrie 2026 / © 2026).
• Docs (docs/leadsheets-and-songbook.md, both skills/bes-song-leadsheets/references/*.md) were updated in the same commit and now match the code — the "Known Limitations" item about /:/:/ not being transformed was correctly removed.

Minor, non-blocking observations (no action needed):

• The PR description already flags the one known pre-existing limitation (split-word chords with no width, e.g. D/F♯E⁴-E) — that's called out as unchanged, so nothing further to do there.
• chords/format = \chordfont\bfseries composes fine since the \IfFontExistsTF false-branch defines \chordfont as \sffamily via \newcommand, and NFSS font-switching commands compose left-to-right as expected.

I wasn't able to execute npm test/npm run lint/npm run typecheck or compile the LaTeX in this sandbox (shell execution requires approval that isn't available here), so I relied on static review plus the screenshots and validation notes already in the PR body. Nothing in the diff looks incorrect or inconsistent.

Since the last review: the lyrics now also use MonoLisa Text when it is installed, with the same \IfFontExistsTF fallback, so CI keeps Latin Modern. The docs and the PR description are updated to match.

@claude review

Claude finished @ioanlucut's task in 46s —— View job

Review

• Gather context (diff since last review: commit a675a96)
• Review MonoLisa Text (lyrics font) changes in template/config
• Review docs/PR description updates for consistency
• Post final review feedback

Since my previous review, the only change is commit a675a96, which extends the existing \IfFontExistsTF chord-font pattern to the lyrics' main font.

Findings: none blocking.

• LaTeX/songbook/bes-songbook.template.txt:39-41: \IfFontExistsTF{MonoLisaText}{\setmainfont{MonoLisaText}[AutoFakeBold=1.5]}{} mirrors the existing MonoLisaCode check for \chordfont (lines 42-44). The empty false-branch is correct — fontspec already defaults \setmainfont to Latin Modern for scrartcl, so CI (without the font) keeps that default, matching the stated intent and the doc update.
• Placement is fine: it runs after \usepackage{fontspec} (line 29, required for \setmainfont/\IfFontExistsTF) and after \setdefaultlanguage{romanian} (line 33), with no ordering conflict with the later extsizes/recalctypearea calls, which don't touch font family selection.
• AutoFakeBold=1.5 on the main font is consistent with the same option already used for \chordfont, so bold lyrics (e.g. verse labels via verses-label-format = \bfseries at bes-songbook.template.txt:77) get the same synthesized-bold treatment already validated for chords in the prior review.
• docs/leadsheets-and-songbook.md:63-65 was updated in the same commit to say "Lyrics and chords are set in MonoLisa... elsewhere, including CI, lyrics use Latin Modern and chords a bold sans serif" — accurately reflects the new fallback behavior, and I confirmed no other doc (including both skills/bes-song-leadsheets/references/*.md files) mentions the old chords-only MonoLisa wording that would now be stale.
• The PR description's validation notes (391→337 pages, etc.) were already given for the combined change; nothing in this incremental commit contradicts them.

I wasn't able to execute npm test/npm run lint/npm run typecheck or compile the LaTeX in this sandbox (shell execution requires approval that isn't available here), so this is a static review. Nothing in the diff looks incorrect or inconsistent.